Add reserved_account_keys module to sdk
#35341
Conversation
jstarry
force-pushed
the
feat/reserved-accounts-mod
branch
from
edb6e7f
to
0c98be2
Compare
jstarry
added
the
feature-gate
| std::collections::HashSet, | ||
| }; | ||
|
|
||
| // Temporary until a zk token program module is added to the sdk |
There was a problem hiding this comment.
Added separately in this PR: #35339
| impl ReservedAccountKeys { | ||
| /// Compute a set of all reserved keys, regardless of if they are active or not | ||
| pub fn active_and_inactive() -> HashSet<Pubkey> { | ||
| RESERVED_ACCOUNT_KEYS |
There was a problem hiding this comment.
any reason to prefer a singleton over making ReservedAccountKeys hold a Vec directly. then Bank can just pass an Arc around.
There was a problem hiding this comment.
The Bank uses an Arc wrapped HashSet. This module lives in solana-sdk because feature set is in solana-sdk but the message types live in solana-program still. So since the message types all operate over a HashSet, the Bank does so too.
There was a problem hiding this comment.
is the Messages referencing this list not the primary bug here? doesn't seem like something we'd want to constrain the solution by. that is, we should invert the test such that this list doesn't need to be public and can be totally encapsulated in runtime/bank
| RESERVED_ACCOUNT_KEYS | ||
| .iter() | ||
| .map(|reserved_key| reserved_key.key) | ||
| .collect() |
There was a problem hiding this comment.
how are these used? if we instead used a HashMap (probably in conjunction with the above change), can we move whatever tests these outputs are used for internally and directly query the map instead of allocating?
There was a problem hiding this comment.
It's only allocated once per bank. What are the keys and values of your proposed HashMap and what does it give us over a HashSet?
There was a problem hiding this comment.
instead of the global Vec, HashMap<Pubkey, Option<Pubkey>>. we get the HashSet-like accessors for free. no allocations other than passing the Arc on to each successor bank
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35341 +/- ##
=========================================
- Coverage 81.7% 81.7% -0.1%
=========================================
Files 834 835 +1
Lines 224244 224368 +124
=========================================
+ Hits 183398 183459 +61
- Misses 40846 40909 +63 |
| impl ReservedAccountKeys { | ||
| /// Compute a set of all reserved keys, regardless of if they are active or not | ||
| pub fn active_and_inactive() -> HashSet<Pubkey> { | ||
| RESERVED_ACCOUNT_KEYS |
There was a problem hiding this comment.
is the Messages referencing this list not the primary bug here? doesn't seem like something we'd want to constrain the solution by. that is, we should invert the test such that this list doesn't need to be public and can be totally encapsulated in runtime/bank
| RESERVED_ACCOUNT_KEYS | ||
| .iter() | ||
| .map(|reserved_key| reserved_key.key) | ||
| .collect() |
There was a problem hiding this comment.
instead of the global Vec, HashMap<Pubkey, Option<Pubkey>>. we get the HashSet-like accessors for free. no allocations other than passing the Arc on to each successor bank
|
This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave |
Problem
No way to introduce new reserved account keys which cannot be write locked by transactions.
Summary of Changes
Add a
ReservedAccountKeysmodule to sdk which facilitates adding feature-gated reserved account keys like new sysvars and new enshrined programs.Note that the new module is not currently used anywhere. It's added here on its own to facilitate faster reviews over smaller patchsets of code. To see the full changes, look here: #34901
Feature Gate Issue: #34899